fix(ColumnLayer): closed shapes with binary data and transition props#10094
Open
charlieforward9 wants to merge 1 commit intovisgl:masterfrom
Open
fix(ColumnLayer): closed shapes with binary data and transition props#10094charlieforward9 wants to merge 1 commit intovisgl:masterfrom
charlieforward9 wants to merge 1 commit intovisgl:masterfrom
Conversation
7 tasks
charlieforward9
changed the title
5 tasks
7 tasks
felixpalmer
reviewed
Mar 30, 2026
| protected _disableFillIndexBuffer() { | ||
| const fillModel = this.state.fillModel!; | ||
| if (fillModel.vertexArray.indexBuffer) { | ||
| // Geometry indices are only for wireframe. Model rebuilds can reattach them. |
Collaborator
There was a problem hiding this comment.
This feels like a bit of a patch for bad behavior elsewhere... is it not possible to avoid the reattachement when the model is rebuilt?
Collaborator
Author
There was a problem hiding this comment.
new approach should be more appropriate
charlieforward9
force-pushed
the
fix/column-geometry
branch
from
0a6258c to
b26d224
Compare
Closes visgl#9463, visgl#10021. ColumnLayer shares a single ColumnGeometry (which carries wireframe indices) between the fill and wireframe models. With binary data and transition props, a buffer-layout rebuild re-applies the gpu geometry to the fill model's new vertex array, re-attaching the wireframe index buffer. Triangle-strip fill is then indexed against wireframe indices and renders with missing faces. Fix the root cause: give the fill model a separate Geometry with no indices (sharing POSITION / NORMAL buffers with the wireframe geometry). The fill model's internal _gpuGeometry.indices is null, so later setBufferLayout to _setGeometryAttributes calls cannot reattach wireframe indices to the fill vertex array. - modules/layers/src/column-layer/column-layer.ts: construct fill-only Geometry without indices in _updateGeometry; drop the per-frame setIndexBuffer(null) workaround in draw(). - modules/aggregation-layers/src/hexagon-layer/hexagon-cell-layer.ts: drop the inline index-buffer reset workaround (no longer needed, resolves the existing TODO). - test/modules/layers/column-layer.spec.ts: regression test asserts the fill model's indexBuffer stays null after a setBufferLayout() rebuild (the trigger that previously leaked wireframe indices).
charlieforward9
force-pushed
the
fix/column-geometry
branch
from
8fdf6ed to
584766e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9463, #10021
Screen.Recording.2026-04-17.at.6.12.02.PM.mov
Background
When using
ColumnLayerwith binary data and transition props, column shapes render with missing faces — sides of each column appear open/incomplete.Root cause
ColumnLayerbuilds oneColumnGeometry(which carries wireframe indices) and passes it to bothfillModelandwireframeModel:fillModel.setGeometry(geometry)stores the geometry infillModel._gpuGeometrywithindicespopulated.Layer._setModelAttributescallsmodel.setBufferLayout(...).setBufferLayoutbuilds a newvertexArrayand re-runs_setGeometryAttributes(this._gpuGeometry), which callssetIndexBuffer(gpuGeometry.indices || null). The wireframe indices get re-attached to the fill model.Trace:
modules/core/src/lib/layer.tsLayer._setModelAttributes→@luma.gl/engineModel.setBufferLayout→_setGeometryAttributes→setIndexBuffer.Fix
Give the fill model a separate
Geometrywith no indices, sharing thePOSITION/NORMALbuffers with the wireframe geometry. The fill model's_gpuGeometry.indicesisnull, so no subsequentsetBufferLayout/_setGeometryAttributespass can re-attach wireframe indices.This fixes the root cause rather than clearing a leaked index buffer every frame in
draw()(addresses @felixpalmer's review feedback on the previous revision).Change list
modules/layers/src/column-layer/column-layer.ts— construct a fill-onlyGeometry(noindices, topologytriangle-strip) in_updateGeometry; drop thesetIndexBuffer(null)workaround.modules/aggregation-layers/src/hexagon-layer/hexagon-cell-layer.ts— drop the inline index-buffer reset workaround (resolves theTODO - this should be handled in ColumnLayer?).test/modules/layers/column-layer.spec.ts— regression test that simulates the buffer-layout rebuild (fillModel.setBufferLayout(fillModel.bufferLayout)) and assertsfillModel.vertexArray.indexBufferstaysnull.Branch state
Rebased onto latest
master(pastv9.3.0-beta.2). Single focused commit. The earlier cross-module TS/build cleanup commit was dropped on rebase — all of those fixes landed on master via #10123, #10141, #10158, and #10201.Render test golden image
A render test case (
column-lnglat-binary-extruded) should be added once the vitest render test migration lands. @chrisgervang — can you help wire up the golden image capture when that's ready?